FIS getAuthToken implementation. #769

ankitaj224 · 2019-09-05T18:02:51Z

No description provided.

…nto fis_auth

…to fis_auth

ankitaj224 · 2019-09-05T18:11:19Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

+  private static <F, T> Continuation<F, T> call(@NonNull Supplier<T> supplier) {
+    return t -> {
+      if (!t.isComplete()) {
+        Tasks.await(t);


@ciarand Instrumented tests fail sometimes because of this line. Its not strictly waiting for the prev Task (getId) to complete. Ends up calling refreshAuthTokenIfNecessary with empty PersistedFidEntry ( pointed you to that code).

Can you please help me to understand what am I missing?

Test failure is alternating.

I don't think this is correct. You're calling Tasks.call(executor, this::getId) but getId is already calling Tasks.call so you're doubly nesting your async operations.

Instead of this:

return Tasks.call(executor, this::getId) .continueWith(call(() -> refreshAuthTokenIfNecessary(forceRefresh)));

I think you mean to do something like this:

return getId().continueWith((idTask) -> refreshAuthTokenIfNecessary(idTask.getResult(), forceRefresh));

NOTE: The call to getResult will throw an Exception if the getId task failed, which will cause the return value of this method (the getAuthToken method) to be a failed Task, which is afaict what we want.

You're also not passing an explicit executor into the .continueWith call though, so the refreshAuthTokenIfNecessary method is getting run on the main thread (!). Given you're not allowed to call Tasks.await on the main thread, that seems like a bug.

Very good catch. You are right. That was the issue :) Fixed it. About the 'Tasks.await' it wasn't a bug until I called Tasks.call(executor, this::getId) . After the change recommended by you, I could not await in the Main Thread. I updated the code to await using the executor. Let me know what you think.

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

...ndroidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

ciarand · 2019-09-05T22:33:09Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

+  private static <F, T> Continuation<F, T> call(@NonNull Supplier<T> supplier) {
+    return t -> {
+      if (!t.isComplete()) {
+        Tasks.await(t);


I don't think this is correct. You're calling Tasks.call(executor, this::getId) but getId is already calling Tasks.call so you're doubly nesting your async operations.

Instead of this:

return Tasks.call(executor, this::getId) .continueWith(call(() -> refreshAuthTokenIfNecessary(forceRefresh)));

I think you mean to do something like this:

return getId().continueWith((idTask) -> refreshAuthTokenIfNecessary(idTask.getResult(), forceRefresh));

NOTE: The call to getResult will throw an Exception if the getId task failed, which will cause the return value of this method (the getAuthToken method) to be a failed Task, which is afaict what we want.

You're also not passing an explicit executor into the .continueWith call though, so the refreshAuthTokenIfNecessary method is getting run on the main thread (!). Given you're not allowed to call Tasks.await on the main thread, that seems like a bug.

2. Renaming InstallationTokenResult TokenExpirationTimestampMillis field to TokenExpirationInSecs

...ations-interop/src/main/java/com/google/firebase/installations/FirebaseInstallationsApi.java

ankitaj224 · 2019-09-06T21:05:56Z

...ndroidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java

@@ -217,4 +225,22 @@ public void testGetId_PersistedFidError_BackendOk() throws InterruptedException
          .isEqualTo(FirebaseInstallationsException.Status.CLIENT_ERROR);
    }
  }
+
+  @Test
+  public void testGetAuthToken_registeredFid_successful() throws Exception {


I just added one test to review the IntDef usage. I will add more tests, once that is reviewed . Thank you.

2. Renaming InstallationTokenResult TokenExpirationTimestampMillis field to TokenExpirationInSecs

…nto fis_auth

...ations-interop/src/main/java/com/google/firebase/installations/FirebaseInstallationsApi.java

...ndroidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

…nto fis_auth

diwu-arete · 2019-09-09T18:29:43Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

@@ -169,6 +180,15 @@ private static boolean persistedFidMissingOrInErrorState(PersistedFidEntry persi
    };
  }

+  @NonNull
+  private <F, T> Continuation<F, T> call(@NonNull Supplier<T> supplier) {


give this method a more specific name?

Done. PTAL.

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

...ations-interop/src/main/java/com/google/firebase/installations/FirebaseInstallationsApi.java

ankitaj224 · 2019-09-12T20:25:19Z

Friendly ping !!

2. Minor code changes as per the offline discussion with Rayo

VinayGuthal · 2019-09-13T18:28:05Z

run ./gradlew :firebase-installations:generateApiTxtFile and commit the changes to fix the api-information bug

…to fis_auth

VinayGuthal · 2019-09-13T18:45:50Z

same thing with :firebase-installations-interop:apiInformation project as well

…to fis_auth

ankitaj224 · 2019-09-13T19:03:25Z

/test new-smoke-tests

ciarand · 2019-09-13T18:21:38Z

...ndroidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java


  @Before
  public void setUp() throws FirebaseInstallationServiceException {
    MockitoAnnotations.initMocks(this);
    FirebaseApp.clearInstancesForTest();
-    executor = new ThreadPoolExecutor(0, 2, 10L, TimeUnit.SECONDS, new SynchronousQueue<>());
+    executor = new ThreadPoolExecutor(0, 4, 10L, TimeUnit.SECONDS, new SynchronousQueue<>());


can you add a comment explaining why you've chosen 4 for this test?

Done. PTAL.

...ndroidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java

ciarand · 2019-09-13T18:23:20Z

...ndroidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java

+      Throwable cause = expected.getCause();
+      assertWithMessage("Exception class doesn't match")
+          .that(cause)
+          .isInstanceOf(FirebaseInstallationsException.class);


Truth has built-in support for unpacking exceptions

assertWithMessage("w/e").that(expected).hasCauseThat().isInstanceOf(FirebaseInstallationsException.class)

Done. Did you also want the status to be changed? I tried but dint find a easy way.

You'd have to create a custom truth subject for that and tbh I don't think it's worth it atm

ciarand · 2019-09-13T18:25:19Z

...ndroidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java

+
+  @Test
+  public void testGetAuthToken_unregisteredFid_fetchedNewTokenFromFIS() throws Exception {
+    when(mockPersistedFid.readPersistedFidEntryValue())


imo this test could do with some comments explaining what you're testing

Done. PTAL.

...ndroidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java

ciarand · 2019-09-13T18:27:21Z

...ndroidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java

+        firebaseInstallations.getAuthToken(FirebaseInstallationsApi.FORCE_REFRESH);
+    Thread.sleep(500);
+    Task<InstallationTokenResult> task2 =
+        firebaseInstallations.getAuthToken(FirebaseInstallationsApi.FORCE_REFRESH);


we shouldn't have to sleep in tests. Can we somehow use the injected to ensure these tasks are run?

I wanted the sleep to ensure task executed in order. But I removed it, instead of validating on both expected values

Why don't they execute in order at the moment? Is it because the executor has 4 threads? Could we use a single threaded executor to make this deterministic?

ciarand · 2019-09-13T18:29:37Z

...ndroidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java

+
+  class ServiceClient extends FirebaseInstallationServiceClient {
+
+    private boolean secondCall;


this should be explained or reworked imo, it's not super obvious what this helper and its test are checking.

Could you use an inline mockito answer (thenAnswer instead of thenThrow / thenReturn) to clarify the goal of the test?

Thanks for the suggestion. I have tried using thenAnswer. PTAL.

firebase-installations/src/main/java/com/google/firebase/installations/AwaitListener.java

ciarand · 2019-09-13T18:31:00Z

firebase-installations/src/main/java/com/google/firebase/installations/AwaitListener.java

+final class AwaitListener implements OnCompleteListener<Void> {
+  private final CountDownLatch mLatch = new CountDownLatch(1);
+
+  public void onSuccess() {


this isn't actually used, right?

Its used. If the status is REGISTERED, instead of waiting for 10s, I am using this method to update the awaitlistener.

FirebaseInstalllations.java line 261

ciarand · 2019-09-13T18:31:59Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

+      @NonNull Supplier<T> supplier, AwaitListener listener) {
+    return t -> {
+      // Waiting for Task that registers FID on the FIS Servers
+      listener.await(10, TimeUnit.SECONDS);


move this to a constant imo

…to fis_auth

ankitaj224 · 2019-09-16T17:05:21Z

/test check-changed

firebase-installations-interop/api.txt

...lations-interop/src/main/java/com/google/firebase/installations/InstallationTokenResult.java

...ndroidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java

ciarand · 2019-09-16T17:25:29Z

...ndroidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java

+      Throwable cause = expected.getCause();
+      assertWithMessage("Exception class doesn't match")
+          .that(cause)
+          .isInstanceOf(FirebaseInstallationsException.class);


You'd have to create a custom truth subject for that and tbh I don't think it's worth it atm

...ndroidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java

ciarand · 2019-09-16T17:28:23Z

...ndroidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java

+    // and verify one task waits for another task to complete.
+
+    doAnswer(
+            new AnswersWithDelay(


I'm not 100% sure here, but I think we're supposed to use the AdditionalAnswers class to create these instead of reaching into the mockito internal package: https://static.javadoc.io/org.mockito/mockito-core/2.9.0/org/mockito/AdditionalAnswers.html

Thanks for the link. I dint realize it was internal package. Updated.

ciarand · 2019-09-16T17:32:19Z

...ndroidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java

+        firebaseInstallations.getAuthToken(FirebaseInstallationsApi.FORCE_REFRESH);
+    Thread.sleep(500);
+    Task<InstallationTokenResult> task2 =
+        firebaseInstallations.getAuthToken(FirebaseInstallationsApi.FORCE_REFRESH);


Why don't they execute in order at the moment? Is it because the executor has 4 threads? Could we use a single threaded executor to make this deterministic?

...ndroidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java

ciarand · 2019-09-16T19:11:29Z

...ndroidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java

@@ -480,10 +476,10 @@ public void testGetAuthToken_multipleCallsForceRefresh_fetchedNewTokenTwice() th
    // As we cannot ensure which task got executed first, verifying with both expected values
    assertWithMessage("Persisted Auth Token doesn't match")
        .that(task1.getResult().getToken())
-        .isAnyOf(TEST_AUTH_TOKEN_3, TEST_AUTH_TOKEN_4);
+        .isEqualTo(TEST_AUTH_TOKEN_3);


...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

ankitaj224 added 7 commits September 5, 2019 10:48

FIS getAuthToken implementation.

c54292e

FIS getAuthToken implementation.

b4b6ba8

Merge branch 'fis_auth' of github.com:firebase/firebase-android-sdk i…

b8bfc1a

…nto fis_auth

Merge branch 'fis_auth' of github.com:firebase/firebase-android-sdk i…

7a2a112

…nto fis_auth

Merge branch 'fis_auth' of github.com:firebase/firebase-android-sdk i…

42c20f6

…nto fis_auth

Merge branch 'fis_sdk' of github.com:firebase/firebase-android-sdk in…

46d936c

…to fis_auth

Merge branch 'fis_sdk' of github.com:firebase/firebase-android-sdk in…

48fd7fe

…to fis_auth

ankitaj224 requested review from ciarand, andirayo and diwu-arete September 5, 2019 18:02

googlebot added the cla: yes Override cla label Sep 5, 2019

google-oss-bot added the size/L label Sep 5, 2019

ankitaj224 commented Sep 5, 2019

View reviewed changes

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java Outdated Show resolved Hide resolved

ciarand suggested changes Sep 5, 2019

View reviewed changes

1. Addressing Ciaran's comments

82fb69e

2. Renaming InstallationTokenResult TokenExpirationTimestampMillis field to TokenExpirationInSecs

ankitaj224 commented Sep 6, 2019

View reviewed changes

...ations-interop/src/main/java/com/google/firebase/installations/FirebaseInstallationsApi.java Outdated Show resolved Hide resolved

ankitaj224 requested a review from ciarand September 6, 2019 21:04

ankitaj224 commented Sep 6, 2019

View reviewed changes

ankitaj224 added 2 commits September 6, 2019 14:48

1. Addressing Ciaran's comments

fe04e15

2. Renaming InstallationTokenResult TokenExpirationTimestampMillis field to TokenExpirationInSecs

Merge branch 'fis_auth' of github.com:firebase/firebase-android-sdk i…

e91cb7e

…nto fis_auth

ciarand suggested changes Sep 6, 2019

View reviewed changes

ankitaj224 added 3 commits September 6, 2019 14:55

Merge branch 'fis_auth' of github.com:firebase/firebase-android-sdk i…

8569952

…nto fis_auth

Merge branch 'fis_auth' of github.com:firebase/firebase-android-sdk i…

b74b163

…nto fis_auth

Updated IntDef usage in firebase-installations

58a30e3

ankitaj224 requested a review from ciarand September 6, 2019 22:15

Using CountDownLatch to await instead of executor.awaitTermination

96e30c2

diwu-arete reviewed Sep 9, 2019

View reviewed changes

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java Show resolved Hide resolved

diwu-arete reviewed Sep 9, 2019

View reviewed changes

...ations-interop/src/main/java/com/google/firebase/installations/FirebaseInstallationsApi.java Show resolved Hide resolved

diwu-arete self-requested a review September 11, 2019 20:54

diwu-arete approved these changes Sep 11, 2019

View reviewed changes

Addressing Di's comments.

91e49bc

1. Handling multiple calls to getAUthToken()

4578880

2. Minor code changes as per the offline discussion with Rayo

google-oss-bot added size/XL and removed size/L labels Sep 13, 2019

ankitaj224 requested a review from diwu-arete September 13, 2019 17:46

diwu-arete approved these changes Sep 13, 2019

View reviewed changes

ankitaj224 added 2 commits September 13, 2019 11:30

Fixing api-information presubmit check.

7d19b50

Merge branch 'fis_sdk' of github.com:firebase/firebase-android-sdk in…

fe04884

…to fis_auth

ankitaj224 added 2 commits September 13, 2019 11:48

Merge branch 'fis_sdk' of github.com:firebase/firebase-android-sdk in…

f1d1d37

…to fis_auth

updating api.txt for firbase-installations-interop

ec65040

ciarand reviewed Sep 13, 2019

View reviewed changes

ankitaj224 added 2 commits September 13, 2019 14:27

Merge branch 'fis_sdk' of github.com:firebase/firebase-android-sdk in…

396d273

…to fis_auth

Addressing Ciaran's comments.

81b4aeb

ankitaj224 requested a review from ciarand September 13, 2019 23:21

Fixing check-changed - GoogleJavaFormat error.

3932d3b

ciarand reviewed Sep 16, 2019

View reviewed changes

Addressing ciaran`s comments

8d4d811

ankitaj224 requested a review from ciarand September 16, 2019 18:58

ciarand approved these changes Sep 16, 2019

View reviewed changes

nit fixes to executor : use unblocking queue.

3f44b77

ankitaj224 merged commit 6a013fa into fis_sdk Sep 16, 2019

rlazo deleted the fis_auth branch October 3, 2019 14:44

firebase locked and limited conversation to collaborators Oct 17, 2019


		class ServiceClient extends FirebaseInstallationServiceClient {

		private boolean secondCall;

FIS getAuthToken implementation. #769

FIS getAuthToken implementation. #769

Uh oh!

Conversation

ankitaj224 commented Sep 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ankitaj224 commented Sep 12, 2019

Uh oh!

VinayGuthal commented Sep 13, 2019

Uh oh!

VinayGuthal commented Sep 13, 2019

Uh oh!

ankitaj224 commented Sep 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ankitaj224 commented Sep 16, 2019

Uh oh!

Uh oh!

Uh oh!